Skip to content

fix: code quality review fixes#8

Merged
eddietejeda merged 5 commits into
mainfrom
fix/code-quality-review
May 20, 2026
Merged

fix: code quality review fixes#8
eddietejeda merged 5 commits into
mainfrom
fix/code-quality-review

Conversation

@eddietejeda
Copy link
Copy Markdown
Contributor

@eddietejeda eddietejeda commented May 20, 2026

Summary

Five separate commits addressing code quality review findings:

  • Bug — build_managed_config now always emits the schema block even when tables=[]. Previously create_database with no tables sent config: {} and the schema name was silently dropped. Includes a regression test.
  • Refactor — removed duplicate obj/schema validation from create_table; _local_table_to_parquet already owns that check with the same message.
  • Test — added missing client.close() in test_result_arrow_poll_handles_accepted_result, matching every other test in that file.
  • Docs — added docstring to _managed_table_synced explaining why synced=False tables return False (intentional: allows retrying an in-progress load without overwrite=True).
  • Nit — added inline comment to the broad except Exception in dtype_from_hotdata_sql_type explaining why it cannot be narrowed (ibis/sqlglot raise several different types depending on which parse step fails).

Test plan

  • All CI jobs pass
  • test_create_database_no_tables_still_sends_schema covers the regression

🤖 Generated with Claude Code

eddietejeda and others added 5 commits May 19, 2026 19:42
… empty

build_managed_config was returning {} when tables=[], silently dropping
the schema name. A call like create_database("db", schema="analytics")
would send config: {} to the API and the schema declaration was lost.

Remove the early return so the schema block is always emitted. Add a
regression test for the empty-tables path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The check was already present in _local_table_to_parquet with the same
condition and message. Having it in create_table too was dead code.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cepted_result

Every other HotdataClient created in this file is closed at the end of
the test. This one was missed, leaving a dangling connection pool.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The method returns False for tables whose synced flag is False, which
lets create_table proceed without overwrite=True while a load is still
in progress. This was intentional but undocumented, making the behavior
look like a bug.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…om_hotdata_sql_type

ibis and sqlglot raise several different exception types depending on
which part of type parsing fails (ValueError, AttributeError, parse
errors internal to sqlglot). Narrowing to a specific type risks missing
one and breaking type discovery for a valid-but-unusual column type.
Add a comment so the broad catch is clearly deliberate.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@eddietejeda eddietejeda merged commit 789c4f5 into main May 20, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant